Give the tools an 8MB stack on msvc - #2815

Thanks for the review. Pushed 505d3fa addressing all three comments — and CI caught a real problem with the original limit, so the value has changed.

1000 was too high. build (windows-latest) exited 0xC00000FD (STATUS_STACK_OVERFLOW) before the limit was ever reached: msvc debug frames are large enough to blow the default 1MB stack first. Ironic, but it is the exact failure this PR is about, so it had to move. (The macOS failure alongside it was just fail-fast cancellation — it had passed.)

> I wonder if real world module will blow through this limit or not?

I measured instead of guessing. Decoding the code section of every module in the spec testsuite plus wabt's own tests — 18952 modules:

Real control flow is very shallow; the only deep thing in the tree is a test written to be deep. The outlier in the wild is the generated code in #2377, which was thousands deep — that one still gets an error, but an error rather than a segfault.

So 128 sits ~1.6x above the deepest module I can find anywhere, and roughly 4x under where msvc debug falls over (~600 bytes/level measured on gcc debug, and msvc is worse). If you would rather trade margin for headroom, raising it is a one-line change — I just would not go near 1000 again without also growing the stack, which is what #2748 does.

One consequence worth naming: BinaryReaderIR::kMaxNestingDepth is 16384, so a binary nested deeper than 128 can now be read but not round-tripped back through the text parser. That gap already existed in the other direction (the text parser used to crash where the binary reader errored cleanly); this makes both ends report an error, just at different depths.

Full test suite is green against a debug ASAN/UBSAN build with all submodules checked out, and scripts/clang-format-diff.sh is clean.

You were right to push on that, the spec tests turn out to say almost nothing useful here. I went and measured real toolchain output instead, taking the max control depth out of wasm2wat's own label annotations:

webp_enc (emscripten)     29
resvg (wasm-pack)        180
sqlite via sql.js (emsc) 282
esbuild (go)            3457

So 128 was too low. Round-tripping sqlite through wasm2wat needs a limit of 320 and resvg needs 192, both of which I would have broken. I have raised it to 512, which covers those with some room and is still half of the 1000 that overflowed msvc debug in CI on this PR.

esbuild is the one worth dwelling on. At 3457 it is deeper than any build I have survives, so wat2wasm already cannot read wasm2wat's own output for it — it faults on Windows and under ASAN. That is this bug on a real module rather than a synthetic one, which is nice motivation, but it also means no limit can rescue that case: 3457 levels is several MB of frames and the tightest thing we build for is a 1MB stack. All the limit does there is turn the segfault into a message.

On the spec link: A3 does list "the nesting depth of structured control instructions" as a dimension implementations may restrict, but it also says up front that where restrictions take the form of numeric limits, "no minimum requirements are given, nor are the limits assumed to be concrete, fixed numbers". So there is deliberately no value in there to copy.

Engines do not seem to have one either. V8's wasm-limits.h has the ones wabt already mirrors (function size, 50000 locals, 1000 params/returns, br_table 65520) but nothing for control depth. That follows from their decoders being iterative — nesting costs heap rather than stack, so there is nothing to defend against. It is specifically recursive text parsers that need this. wasm-tools is the closest comparison and they went structural instead: the wast crate deliberately avoids call-stack recursion when parsing expressions, on the grounds that it is parsing user input that risks blowing the stack, and exposes parser.depth() so callers can impose a limit themselves.

That is probably the honest long-term answer for wabt too, if deeply nested modules should actually parse rather than just fail cleanly. Short of that, the limit is capped by the smallest stack we build for, so I would not go much above 512 without raising the stack as well — which is what #2748 is doing on the Windows side. Happy to pick a different number if you have a preference.

CI came back and 512 still faults on windows, so there is a second half to this that I had wrongly assumed was optional.

msvc's default stack is 1MB. That is not enough to reach the limit, so deeply nested input dies before the parser gets to say anything — the check never runs. Picking a limit that does fit in 1MB is not really a way out either: it would have to be somewhere under 256, and real modules need more than that, so wat2wasm would start rejecting input that parses fine today on Linux and macOS. Trading a crash on Windows for a regression everywhere else seemed like the wrong deal.

So I have added the linker flag to ask for the 8MB stack that Linux and macOS already give us. That is the same change #2748 makes, for the same reason — I have said so in the commit and in the comment next to it. If you would rather land #2748 first I will happily drop it from here and rebase; it is one target_link_options line and the conflict should be trivial either way.

That does mean the two pieces are less independent than I claimed earlier: the limit stops the recursion running off the end of the stack, but on msvc it needs the bigger stack to be reachable at all. Sorry for the earlier framing, I did not have the Windows data then.

For reference, where the numbers landed: 512 covers sqlite (needs 320) and resvg (needs 192) with room, and sits well under the ~13000 levels an 8MB stack allows at the ~600 bytes per level I measured. macOS passed 512 before it got cancelled by the windows failure, so it was only ever msvc that objected.

I think its fine to land the stack size bump as a part of this change since #2748 includes other changes that we still working through

Can you suggest a more concise message to be used for the final squashed commit?

Reference dropped, thanks — and good to know about #2748.

For the squash message:

Limit instruction nesting depth in the wast parser

The parser is recursive descent, so deeply nested but otherwise valid
text overflowed the stack rather than reporting an error. Add a nesting
limit, and raise the msvc stack to 8MB since its 1MB default is too
small to reach that limit.

Fixes #2377.

After chatting with @tlively about about it does seems like real world module can have a very deep level nesting dure the br_table instruction. Apparently a swith/case in C can lower to a br_table with a nesting depth propositional the number of cases.

So I'm not sure what to do here.. maybe the real solution is to refactor to avoid the use of the native stack? But that seems like a lot of work. I'm temped to maybe just do nothing (aside from maybe increate the windows stack size).

Is there much point exiting early before full stack exhaustion is reached?

That matches what I am seeing. In esbuild the deep part is exactly that shape, long runs of nested blocks wrapping br_tables, and it bottoms out at 3457.

On whether exiting early is worth it: for real modules, no. With no nesting limit at all, a debug build handles 14000 nested blocks on an 8MB stack, and esbuild's own text reads back fine:

stack 1024 KB  CRASH
stack 8192 KB  ok

That is the whole 1.9GB .wat, with nothing in the parser stopping it. So the stack size is what makes real input work, not the limit. Mine at 512 was tuned to fit msvc's 1MB default, which is exactly the constraint the stack bump removes, so it was solving the wrong half.

I have dropped the limit. The PR is now just the linker flag, and I have retitled it to match.

For the record on what a limit would have bought, since it is not nothing: libwabt gets embedded, and stack exhaustion is an uncatchable crash in whatever process links it, while an error can be handled. It would also make behaviour the same across build configurations rather than depending on how much stack you happen to have. But that only holds if the number sits far above anything real, and as a tuned value it is just a way to reject valid modules, which is the thing you and @tlively are pointing at. Not worth it here.

This does not make the recursion safe in general, to be clear. Deep enough input still exhausts whatever stack you give it. Doing that properly means not using the native stack, the way the wast crate does, and I agree that is a lot of work and should not hold up the 8MB.

Also correcting myself from earlier in the thread: I said esbuild could not be read at all. With 8MB it can. That was only true at the smaller stack.

Note that if we embed libwabt inside another executable then the approach decribed in the initial PR also doesn't work because we (a) we don't know how deep we are in the stack when we start parsing and (b) We don't know how much stack space the embedding program was linking with. So another approach would be needed anyway I think, something that can explicitly know the bounds of the stack I guess? But i'm not aware of portable solution there.